internal/exec/utils.go (1)

322-322: Consider providing extra context in returned errors.

Returning the error as-is is perfectly fine for now, but adding more context can help with debugging in complex scenarios.

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

internal/exec/utils.go (1)

322-322: Consider error wrapping for better traceability
Although returning the error here is correct, consider adding more context to make debugging easier. Silently propagating the error can make it tougher to trace where it originated.

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

pkg/store/aws_ssm_param_store.go (2)

32-50: Recommended check for AWS session reuse.

The code properly loads AWS configuration and sets up an SSM client. Consider reusing or caching the session if performance across multiple calls becomes a concern.

---

53-75: Validate parameter value is not empty.

While it is correct to only handle string inputs, a quick check for blank or overly large values may help catch invalid usage.

pkg/store/store.go (1)

9-10: Clear extension point for store creation.

StoreFactory provides a flexible mechanism for injecting different store backends. Infuse helpful logging or metrics if needed.

pkg/store/config.go (1)

12-14: Surface mapstructure errors gracefully.

It might be helpful to wrap the error with more context about which config field is causing issues.

pkg/hooks/hooks.go (1)

18-28: Consider adding validation for hook events

The Hook structure looks solid, but consider adding validation to ensure that only valid event types from HookType constants are used in the Events slice.

 type Hook struct {
 	Events  []string `yaml:"events"`
 	Command string   `yaml:"command"`
 
 	// Dynamic command-specific properties
 
 	// store command
 	Name   string                 `yaml:"name,omitempty"`   // for store command
 	Values map[string]interface{} `yaml:"values,omitempty"` // for store command
+
+	// Consider adding:
+	// ValidateEvents() error
 }

pkg/store/registry.go (1)

7-36: Consider enhancing store type validation and error handling

The store registry implementation is solid, but could benefit from a few improvements:

1. Consider using constants for store types instead of string literals
2. Add validation for required fields in store configurations before processing
3. Consider implementing a cleanup method for proper resource management

+// Store types
+const (
+    StoreTypeSSM      = "aws-ssm-parameter-store"
+    StoreTypeInMemory = "in-memory"
+)

 func NewStoreRegistry(config *StoresConfig) (StoreRegistry, error) {
     registry := make(StoreRegistry)
+    if config == nil {
+        return nil, fmt.Errorf("store config cannot be nil")
+    }
     for key, storeConfig := range *config {
+        if err := validateStoreConfig(storeConfig); err != nil {
+            return nil, fmt.Errorf("invalid store config for %s: %w", key, err)
+        }
         switch storeConfig.Type {
-        case "aws-ssm-parameter-store":
+        case StoreTypeSSM:
             // ... rest of the code
-        case "in-memory":
+        case StoreTypeInMemory:
             // ... rest of the code

pkg/store/in_memory_store.go (1)

22-28: Consider adding nil value validation in Set method

The Set method should validate the input parameters to prevent potential panics. This aligns with the PR's goal of panic prevention.

 func (m *InMemoryStore) Set(key string, value interface{}) error {
+	if key == "" {
+		return fmt.Errorf("key cannot be empty")
+	}
 	m.mu.Lock()
 	defer m.mu.Unlock()
 	m.data[key] = value
 	return nil
 }

internal/exec/yaml_func_store.go (1)

12-41: Strengthen error handling and remove unused parameter

The implementation looks solid, but there are a few improvements to consider:

1. The currentStack parameter is unused
2. Consider adding more context to error messages

-func processTagStore(cliConfig schema.CliConfiguration, input string, currentStack string) any {
+func processTagStore(cliConfig schema.CliConfiguration, input string) any {
 	log.Debug("Executing Atmos YAML function store", "input", input)

 	str, err := getStringAfterTag(input, u.AtmosYamlFuncStore)
 	if err != nil {
-		u.LogErrorAndExit(cliConfig, err)
+		u.LogErrorAndExit(cliConfig, fmt.Errorf("failed to process store tag: %w", err))
 	}

pkg/utils/yaml_utils.go (1)

11-17: Well structured constants, warrior! Consider adding documentation.

The constants are well-organized and improve maintainability. A brief comment for each constant explaining its purpose would make it even better.

Consider adding specific documentation for each constant:

 const (
 	// Atmos YAML functions
-	AtmosYamlFuncExec            = "!exec"
-	AtmosYamlFuncStore           = "!store"
-	AtmosYamlFuncTemplate        = "!template"
-	AtmosYamlFuncTerraformOutput = "!terraform.output"
+	// AtmosYamlFuncExec executes shell commands
+	AtmosYamlFuncExec            = "!exec"
+	// AtmosYamlFuncStore manages store operations
+	AtmosYamlFuncStore           = "!store"
+	// AtmosYamlFuncTemplate processes template strings
+	AtmosYamlFuncTemplate        = "!template"
+	// AtmosYamlFuncTerraformOutput retrieves terraform outputs
+	AtmosYamlFuncTerraformOutput = "!terraform.output"
 )

pkg/store/in_memory_store_test.go (3)

8-16: Strong test for store creation, warrior!

The test effectively verifies store initialization. Consider adding a test case for configuration validation.

Consider adding a test case with invalid configuration:

func TestNewMemoryStoreWithInvalidConfig(t *testing.T) {
    invalidConfig := map[string]interface{}{
        "invalid": "config",
    }
    store, err := NewInMemoryStore(invalidConfig)
    if err == nil {
        t.Error("NewMemoryStore() expected error with invalid config")
    }
}

---

18-64: Well-structured test cases for Set operations!

The table-driven tests cover various data types effectively. Consider adding error cases and concurrent access tests.

Consider adding:

1. Test case for nil value
2. Concurrent access test to verify thread safety

func TestMemoryStoreConcurrentAccess(t *testing.T) {
    store := &InMemoryStore{
        data: make(map[string]interface{}),
    }
    const goroutines = 10
    var wg sync.WaitGroup
    wg.Add(goroutines)
    
    for i := 0; i < goroutines; i++ {
        go func(id int) {
            defer wg.Done()
            key := fmt.Sprintf("key-%d", id)
            err := store.Set(key, id)
            if err != nil {
                t.Errorf("Concurrent Set() error = %v", err)
            }
        }(i)
    }
    wg.Wait()
}

---

66-108: Solid test coverage for Get operations!

The tests handle both existing and non-existing keys well. Consider adding type assertion tests.

Consider adding test cases for type assertions:

{
    name:      "type assertion",
    key:       "number-key",
    value:     42,
    want:      42,
    wantError: false,
},

internal/exec/yaml_func_terraform_output.go (1)

Line range hint 52-63: Effective caching mechanism!

The cache implementation using sync.Map is thread-safe and prevents redundant terraform output calls.

Consider implementing a cache expiration mechanism for long-running processes to prevent stale data.

pkg/store/aws_ssm_param_store_test.go (2)

36-93: Consider adding more edge cases to strengthen test coverage.

The current test cases cover the basic scenarios well. Consider adding tests for:

• Empty key/value
• Very long parameter values (AWS SSM has size limits)
• Special characters in parameter names

---

95-148: Add test cases for common SSM parameter retrieval scenarios.

Consider adding test cases for:

• Parameter not found
• Parameter with different types (String, StringList, SecureString)
• Decryption failures for SecureString parameters

pkg/config/utils.go (1)

491-502: Clean implementation of store configuration processing!

The function is well-structured and handles errors appropriately. Consider enhancing the error context for better debugging.

Consider wrapping the error with additional context:

 func processStoreConfig(cliConfig *schema.CliConfiguration) error {
     log.Debug("processStoreConfig", "cliConfig.StoresConfig", fmt.Sprintf("%v", cliConfig.StoresConfig))
 
     storeRegistry, err := store.NewStoreRegistry(&cliConfig.StoresConfig)
     if err != nil {
-        return err
+        return fmt.Errorf("failed to create store registry: %w", err)
     }
     cliConfig.Stores = storeRegistry
 
     return nil
 }

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

Learnt from: aknysh
PR: cloudposse/atmos#863
File: internal/exec/yaml_func_terraform_output.go:34-38
Timestamp: 2024-12-17T07:08:41.288Z
Learning: In the `processTagTerraformOutput` function within `internal/exec/yaml_func_terraform_output.go`, parameters are separated by spaces and do not contain spaces. Therefore, using `strings.Fields()` for parsing is acceptable, and there's no need to handle parameters with spaces.

#!/bin/bash
# Check for potential parameter parsing edge cases
rg -l "!terraform\.output\s+\S+\s+\S+\s+\S+" | while read -r file; do
    echo "Checking $file for terraform.output usage:"
    rg "!terraform\.output\s+\S+\s+\S+\s+\S+" "$file"
done

#!/bin/bash
# Check the implementation of terraform.output function
ast-grep --pattern 'func terraformOutput($$$) $$$'

# Check for error handling and validation
rg "terraformOutput" -A 10 -B 10

#!/bin/bash
# Search for error handling patterns in processStoreConfig
ast-grep --pattern 'func processStoreConfig($config) $_ {
  $$$
  if err != nil {
    $$$
  }
  $$$
}'

#!/bin/bash
# Let's try a different approach to find processStoreConfig implementation
rg -U "func processStoreConfig" -A 10